feat: Add logging to explain reasons for Comet not being able to run a query stage natively #397

andygrove · 2024-05-07T18:24:24Z

Which issue does this PR close?

Closes #385

Rationale for this change

As a user, I would like to understand why my query stages are not running natively.

What changes are included in this PR?

New config + logging

How are these changes tested?

Tested manually:

scala> spark.read.parquet("/tmp/test").createOrReplaceTempView("t1")
24/05/07 10:55:16 INFO src/lib.rs: Comet native library initialized
24/05/07 10:55:16 WARN CometSparkSessionExtensions$CometExecRule: Comet cannot execute this plan natively because Execute CreateViewCommand is not supported

scala> spark.sql("select * from t1 where cast(cast(a as double) as decimal(10,2)) > 5").show
24/05/07 10:55:29 WARN CometSparkSessionExtensions$CometExecRule: Comet cannot execute this plan natively because:
	- Comet does not guarantee correct results for cast from DoubleType to DecimalType(10,2) with timezone Some(America/Denver) and evalMode LEGACY (No overflow check). To enable all incompatible casts, set spark.comet.cast.allowIncompatible=true
	- CollectLimit is not supported
+---+
|  a|
+---+
|  9|
|  8|
|  6|
|  7|
+---+

common/src/main/scala/org/apache/comet/CometConf.scala

andygrove · 2024-05-07T19:01:38Z

@parthchandra How does this look?

parthchandra

LGTM. Just some observations.

parthchandra · 2024-05-07T19:38:41Z

common/src/main/scala/org/apache/comet/CometConf.scala

@@ -276,6 +276,14 @@ object CometConf {
      .booleanConf
      .createWithDefault(false)

+  val COMET_EXPLAIN_FALLBACK_ENABLED: ConfigEntry[Boolean] =


Do we really need to make this configurable?

I think a config should be good. Sometimes it might be verbose.

parthchandra · 2024-05-07T19:41:09Z

spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala

@@ -734,6 +734,22 @@ class CometSparkSessionExtensions
      } else {
        var newPlan = transform(plan)

+        // if the plan cannot be run natively then explain why (when appropriate config is enabled)


Given that ExecRule is going to be invoked multiple times as planning proceeds, this may get logged multiple times for the same plan. Perhaps that is why we need to keep this configurable.

That is one reason, yes. The logging could get verbose.

I would assume that this feature is useful in development but less so in production since we have the integration with Spark's explain for that.

viirya · 2024-05-07T23:23:45Z

spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala

@@ -734,6 +734,22 @@ class CometSparkSessionExtensions
      } else {
        var newPlan = transform(plan)

+        // if the plan cannot be run natively then explain why (when appropriate config is enabled)
+        if (CometConf.COMET_EXPLAIN_FALLBACK_ENABLED.get() && !isCometNative(newPlan)) {


Comet could only trigger native execution for partial query plan. This check isCometNative(newPlan) only applies for the top operator.

andygrove · 2024-05-08T20:20:31Z

I am moving this to draft for now while I test with TPC-H queries and make some improvements

…-datafusion-comet into explain-fallback-reasons

andygrove · 2024-05-13T13:12:26Z

@viirya @parthchandra This PR is ready for another review. I simplified the logic and removed the check to see if the top level operator is native or not. Instead, if we have recorded any info using withInfo then that info will be displayed when the config is enabled. There is an example in the documentation as part of this PR.

kazuyukitanimura · 2024-05-13T19:17:32Z

spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala

+        // if the plan cannot be run fully natively then explain why (when appropriate
+        // config is enabled)
+        if (CometConf.COMET_EXPLAIN_FALLBACK_ENABLED.get()) {
+          new ExtendedExplainInfo().extensionInfo(newPlan) match {


Unrelated, but I feel ExtendedExplainInfo can be object

Yes, I had the same thought

It is possible that it has to be a class due to the way this integrates into Spark, but I am not sure.

Just seeing this. I don't think Spark prevents us from this being an object. I'll take care of this. #452

kazuyukitanimura · 2024-05-13T19:17:57Z

spark/src/main/scala/org/apache/comet/ExtendedExplainInfo.scala

@@ -45,7 +45,7 @@ class ExtendedExplainInfo extends ExtendedExplainGenerator {
    }
  }

-  private def extensionInfo(node: TreeNode[_]): Set[String] = {
+  def extensionInfo(node: TreeNode[_]): Set[String] = {


Perhaps private[comet] instead of public?

Thanks. I made that change.

andygrove · 2024-05-13T19:32:21Z

@viirya I have 2 approvals but need a committer approval to be able to merge.

viirya · 2024-05-13T19:39:27Z

Looks good to me. Thanks @andygrove

…a query stage natively (apache#397) (cherry picked from commit 3808306)

andygrove added 3 commits May 7, 2024 10:31

Add logging to show fallback reasons

19472ed

refactor

07c7784

revert a change

7becda1

andygrove marked this pull request as draft May 7, 2024 18:27

andygrove commented May 7, 2024

View reviewed changes

common/src/main/scala/org/apache/comet/CometConf.scala Outdated Show resolved Hide resolved

andygrove added 5 commits May 7, 2024 12:29

revert a change

26cbb1c

make format

4f2c26c

rename config

14d95f1

generate configs

5c21783

update description

eb9c21c

andygrove marked this pull request as ready for review May 7, 2024 19:00

andygrove requested a review from viirya May 7, 2024 19:01

parthchandra approved these changes May 7, 2024

View reviewed changes

update docs

22409f4

viirya reviewed May 7, 2024

View reviewed changes

andygrove marked this pull request as draft May 8, 2024 20:20

andygrove added 4 commits May 13, 2024 06:43

remove check for top level operator being native

8e800ec

Merge remote-tracking branch 'apache/main' into explain-fallback-reasons

b8566fe

fix message format and update example in docs

bf87ff7

Merge branch 'explain-fallback-reasons' of github.com:andygrove/arrow…

0dee9c7

…-datafusion-comet into explain-fallback-reasons

andygrove marked this pull request as ready for review May 13, 2024 13:11

kazuyukitanimura approved these changes May 13, 2024

View reviewed changes

address feedback

78043bd

viirya approved these changes May 13, 2024

View reviewed changes

trigger build

af39640

andygrove merged commit 3808306 into apache:main May 14, 2024
40 checks passed

andygrove deleted the explain-fallback-reasons branch May 14, 2024 04:10

himadripal pushed a commit to himadripal/datafusion-comet that referenced this pull request Sep 7, 2024

feat: Add logging to explain reasons for Comet not being able to run …

7f7416d

…a query stage natively (apache#397) (cherry picked from commit 3808306)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add logging to explain reasons for Comet not being able to run a query stage natively #397

feat: Add logging to explain reasons for Comet not being able to run a query stage natively #397

andygrove commented May 7, 2024

andygrove commented May 7, 2024

parthchandra left a comment

parthchandra May 7, 2024

viirya May 13, 2024

parthchandra May 7, 2024

andygrove May 7, 2024

andygrove May 7, 2024

viirya May 7, 2024

andygrove commented May 8, 2024

andygrove commented May 13, 2024

kazuyukitanimura May 13, 2024

andygrove May 13, 2024

andygrove May 13, 2024

parthchandra May 20, 2024

kazuyukitanimura May 13, 2024

andygrove May 13, 2024

andygrove commented May 13, 2024

viirya commented May 13, 2024

feat: Add logging to explain reasons for Comet not being able to run a query stage natively #397

feat: Add logging to explain reasons for Comet not being able to run a query stage natively #397

Conversation

andygrove commented May 7, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

andygrove commented May 7, 2024

parthchandra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andygrove commented May 8, 2024

andygrove commented May 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andygrove commented May 13, 2024

viirya commented May 13, 2024